Fix MDM SSO callback 'missing profile' error for Android enrollment#45046
Open
sharon-fdm wants to merge 4 commits into
Open
Fix MDM SSO callback 'missing profile' error for Android enrollment#45046sharon-fdm wants to merge 4 commits into
sharon-fdm wants to merge 4 commits into
Conversation
…45024) When an Android device enrolls via SSO (OTA enrollment), the callback handler tried to fetch an Apple DEP automatic enrollment profile that doesn't exist when only Android MDM is configured. Add an early return for OTA enrollments that skips the DEP profile fetch, matching the existing guard pattern for account-driven enrollments.
Adds TestOTAEnrollSSOWithoutAppleDEPProfile which verifies the full OTA enrollment SSO flow succeeds even when no Apple DEP automatic enrollment profile exists (simulating an Android-only instance). Also adds LoginOTAEnrollSSOUser test helper that drives the complete OTA SSO flow starting from GET /enroll through SAML IdP login to the callback.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.
Tip: disable this comment in your organization's Code Review settings.
Contributor
|
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR fixes an Android enrollment SSO regression by introducing SSO initiator constants and making the MDM callback handler conditional on enrollment flow type. The change adds four exported constants to identify enrollment flows (OTA enroll, setup experience, account-driven enroll, Apple MDM SSO), updates the SSO initiation and callback logic to dispatch based on these constants instead of hard-coded strings, and conditionally skips automatic DEP profile lookup for OTA flows. Frontend and Orbit client code are wired to use the constants, a test helper implements end-to-end SAML OTA enrollment, and a regression test verifies the callback succeeds without DEP profiles on Android-only instances. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/service/testing_client.go (1)
489-513: 💤 Low valueOptional: extract the shared SAML POST-to-callback flow to reduce duplication.
Steps 3-5 (lines 489–513) are nearly identical to
loginSSOUserWithBodylines 592–618 — the same IdP form-submit, SAMLResponse regex extraction, and callback POST. The only structural difference between the two helpers is how the IdP URL is obtained (via/enrollredirect here vs. Fleet/api/v1/fleet/mdm/ssoinitiation inloginSSOUserWithBody).Extracting a private helper that accepts an already-known IdP URL and drives the SAML credential submission + callback POST would eliminate the duplication:
♻️ Suggested refactor sketch
+// doSAMLFlow drives the IdP credential submission and Fleet SSO callback given +// an already-resolved IdP URL. The cookie jar on client must carry any session +// cookie set before the call. Returns the callback HTTP response. +func (ts *withServer) doSAMLFlow(client *http.Client, idpURL, callbackPath, username, password string) *http.Response { + t := ts.s.T() + resp, err := client.Get(idpURL) + require.NoError(t, err) + parsed, err := url.Parse(resp.Header.Get("Location")) + require.NoError(t, err) + data := url.Values{ + "username": {username}, + "password": {password}, + "AuthState": {parsed.Query().Get("AuthState")}, + } + resp, err = client.PostForm(parsed.Scheme+"://"+parsed.Host+parsed.Path, data) + require.NoError(t, err) + defer resp.Body.Close() + body, err := io.ReadAll(resp.Body) + require.NoError(t, err) + re := regexp.MustCompile(`name="SAMLResponse" value="([^\s]*)" />`) + matches := re.FindSubmatch(body) + require.NotEmptyf(t, matches, "callback HTML doesn't contain a SAMLResponse value, got body: %s", body) + q := url.QueryEscape(string(matches[1])) + cbResp, err := client.Post(ts.server.URL+callbackPath+"?SAMLResponse="+q, "application/x-www-form-urlencoded", nil) + require.NoError(t, err) + return cbResp +}Then
LoginOTAEnrollSSOUserbecomes:- // Step 2: Follow IdP redirect to get the login page - resp, err = client.Get(idpURL) - require.NoError(t, err) - // Step 3-5: ...identical block... + return ts.doSAMLFlow(client, idpURL, "/api/v1/fleet/mdm/sso/callback", username, password)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/service/testing_client.go` around lines 489 - 513, Steps 3–5 duplicate logic across LoginOTAEnrollSSOUser and loginSSOUserWithBody; extract that flow into a private helper (e.g., submitSAMLFromIdP) that takes the IdP URL (string), an http.Client instance, credentials (username, password), and the callback base/URL, then performs the form POST to the IdP (reusing client.PostForm), reads the response body, runs the existing regexp.MustCompile(`name="SAMLResponse" value="([^\s]*)" />`) to extract the SAMLResponse, and finally posts the SAMLResponse to the Fleet callback endpoint (reusing client.Post). Replace the duplicated blocks in LoginOTAEnrollSSOUser and loginSSOUserWithBody with calls to this helper, preserving error handling/assertions (require.NoError/require.NotEmptyf) semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@server/service/testing_client.go`:
- Around line 489-513: Steps 3–5 duplicate logic across LoginOTAEnrollSSOUser
and loginSSOUserWithBody; extract that flow into a private helper (e.g.,
submitSAMLFromIdP) that takes the IdP URL (string), an http.Client instance,
credentials (username, password), and the callback base/URL, then performs the
form POST to the IdP (reusing client.PostForm), reads the response body, runs
the existing regexp.MustCompile(`name="SAMLResponse" value="([^\s]*)" />`) to
extract the SAMLResponse, and finally posts the SAMLResponse to the Fleet
callback endpoint (reusing client.Post). Replace the duplicated blocks in
LoginOTAEnrollSSOUser and loginSSOUserWithBody with calls to this helper,
preserving error handling/assertions (require.NoError/require.NotEmptyf)
semantics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 267bf5b9-2884-4e40-972e-0d14887c9efc
📒 Files selected for processing (4)
changes/45024-android-sso-missing-profileee/server/service/mdm.goserver/service/integration_mdm_test.goserver/service/testing_client.go
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes an SSO callback failure during OTA enrollment by skipping Apple DEP profile lookup when enrolling via /enroll?, allowing Android/BYOD SSO enrollment to succeed on instances without Apple MDM configured.
Changes:
- Add an early return in
mdmSSOHandleCallbackAuthfor OTA enrollment callbacks (/enroll?) to avoid Apple DEP profile requirements. - Add an integration regression test covering OTA enrollment SSO without any Apple DEP profile present.
- Add a testing client helper to execute the full OTA enrollment SSO flow via
/enroll?enroll_secret=....
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| server/service/testing_client.go | Adds a helper to drive the OTA enrollment SSO flow end-to-end in integration tests. |
| server/service/integration_mdm_test.go | Adds regression test ensuring OTA enrollment SSO succeeds without Apple DEP profile. |
| ee/server/service/mdm.go | Skips Apple DEP profile access for /enroll? OTA SSO callbacks. |
| changes/45024-android-sso-missing-profile | Adds changelog entry for the fixed Android SSO “missing profile” error. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Replace raw string literals "ota_enroll", "setup_experience", and "account_driven_enroll" with named constants in fleet.SSO* to prevent typos and missed cases. Constants are defined in server/fleet/app.go so they can be imported by all packages including orbit.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #45046 +/- ##
==========================================
+ Coverage 66.77% 66.79% +0.02%
==========================================
Files 2718 2720 +2
Lines 218795 219037 +242
Branches 10625 10625
==========================================
+ Hits 146100 146314 +214
- Misses 59531 59551 +20
- Partials 13164 13172 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
CI Feedback 🧐A test triggered by this PR failed. Here is an AI-generated analysis of the failure:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #45024
Summary
"missing profile: missing profile"error when an Android device enrolls via SSO (OTA enrollment) on a Fleet instance that does not have Apple MDM configured."ota_enroll","setup_experience","account_driven_enroll") into named constants (fleet.SSOInitiatorOTAEnroll, etc.) to prevent typos and missed cases — which is the class of bug that caused this issue.Code walkthrough
The bug
The bug is in
ee/server/service/mdm.goinmdmSSOHandleCallbackAuth().The flow:
/enroll?enroll_secret=xxx→ frontend callsInitiateMDMSSOwith initiator"ota_enroll"(server/service/frontend.go:248)MDMSSOCallback→ callsmdmSSOHandleCallbackAuthoriginalURL == appleMDMAccountDrivenEnrollmentUrl) → no match for OTAInitiator != "setup_experience"→ true for"ota_enroll"→ enters the blockgetAutomaticEnrollmentProfile()→ returnsnilbecause no Apple MDM is configureddepProf == nil→ returns"missing profile"errorNote that
MDMSSOCallback(the caller) already has a guard at line 931 that correctly skips the Apple MDM verification for/enroll?paths:But
mdmSSOHandleCallbackAuthwas missing the equivalent guard — it unconditionally tried to fetch the Apple DEP profile for any non-setup_experienceinitiator.The fix
Adds an early return for OTA enrollments (where
originalURLstarts with/enroll?), matching the existing pattern for account-driven enrollments right above it. OTA enrollments don't use the Apple DEP profile token.The refactor
Replaced all raw initiator string literals across the backend with named constants defined in
server/fleet/app.go:fleet.SSOInitiatorOTAEnroll"ota_enroll"/enrollpage (Android, BYOD iPhone/iPad)fleet.SSOInitiatorSetupExperience"setup_experience"fleet.SSOInitiatorAccountDrivenEnroll"account_driven_enroll"Constants are in
server/fleet/(notserver/sso/) so orbit can import them without pulling in Redis dependencies.Files changed:
ee/server/service/mdm.go— 6 string replacements (switch cases + comparisons)server/service/frontend.go— 1 replacementorbit/cmd/orbit/orbit.go— 1 replacementserver/service/testing_client.go— 1 replacementserver/service/integration_mdm_test.go— 1 replacementLocal reproduction
Setup
build/fleet serve --dev --dev_licensedocker compose upentity_id: mdm.test.com, SimpleSAML IdP atlocalhost:9080)enable_end_user_authentication: truedirectly in DB (API blocks this without Apple MDM — matches customer state)Steps
GET https://localhost:8080/enroll?enroll_secret=test_enroll_secret→ 303 redirect to SimpleSAML IdPsso_user, pass:user123#)POST https://localhost:8080/api/v1/fleet/mdm/sso/callbackwith the SAMLResponseBefore fix
After fix
No errors. Successful redirect back to the enrollment page with the enrollment reference.
Integration test
Added
TestOTAEnrollSSOWithoutAppleDEPProfilewhich:/enroll→ SAML IdP login → callback)/enroll?...withenrollment_referenceandinitiator=ota_enroll(not?error=true)Confirmed the test fails without the fix (
err="missing profile: missing profile") and passes with the fix.Also added a
LoginOTAEnrollSSOUsertest helper that drives the complete OTA SSO flow starting fromGET /enrollthrough SAML IdP login to the callback, using a single cookie jar.Test plan
/enroll?path)Initiator == "setup_experience")Summary by CodeRabbit